Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openai[patch],azure-openai[patch]: Update legacy classes to respect the apiKey field #5714

Merged
merged 4 commits into from
Jun 11, 2024

Conversation

scottforte
Copy link
Contributor

No description provided.

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jun 10, 2024
Copy link

vercel bot commented Jun 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 11, 2024 8:26pm
langchainjs-docs ✅ Ready (Inspect) Visit Preview Jun 11, 2024 8:26pm

@dosubot dosubot bot added the auto:improvement Medium size change to existing code to handle new use-cases label Jun 10, 2024
@@ -86,7 +86,9 @@ export class OpenAIModerationChain
super(fields);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there! I've flagged this PR for your review as it includes a change that explicitly accesses an environment variable using the getEnvironmentVariable function. Please take a look and ensure it aligns with our environment variable handling practices.

@@ -267,7 +267,9 @@ export class AzureChatOpenAI
getEnvironmentVariable("AZURE_OPENAI_API_DEPLOYMENT_NAME");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there! 👋 I've reviewed the code and noticed that the recent change explicitly accesses environment variables using getEnvironmentVariable. I've flagged this for your review to ensure it aligns with our best practices. Let me know if you have any questions!

@@ -62,7 +62,9 @@ export class AzureOpenAIEmbeddings
getEnvironmentVariable("AZURE_OPENAI_API_ENDPOINT");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there! I've reviewed the code and flagged a change in the diff for you to review. It looks like the code is explicitly accessing and requiring an environment variable, so it's important to double-check this change. Let me know if you need any further assistance!

@@ -135,7 +135,9 @@ export class AzureOpenAI<
getEnvironmentVariable("AZURE_OPENAI_API_DEPLOYMENT_NAME");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey team, just a heads up that I've flagged a change in the PR for review. The added line getEnvironmentVariable("OPENAI_API_KEY"); explicitly accesses an environment variable, so it's worth double-checking to ensure everything is handled correctly.

@@ -157,7 +157,9 @@ export class OpenAIChat
super(fields ?? {});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey team, I've flagged this change for review as it appears to add, access, and read environment variables using getEnvironmentVariable and process.env. It's important to ensure that environment variable handling is secure and follows best practices.

@jacoblee93
Copy link
Collaborator

jacoblee93 commented Jun 10, 2024

Thank you for this - CC @bracesproul let's make sure we add this to the new standardized tests.

Thanks, still good to add, but note that all these classes are deprecated (including the Azure OpenAI package)

You should use ChatOpenAI instead for the newer OpenAI models, since they are chat models and not LLMs:

https://js.langchain.com/v0.2/docs/concepts#chat-models

@scottforte
Copy link
Contributor Author

ChatOpenAI

Hey @jacoblee93 the page here does not show deprecated:
https://js.langchain.com/v0.2/docs/integrations/llms/openai

Does that UI page need to be updated as well?

@jacoblee93
Copy link
Collaborator

OpenAI LLMs are not deprecated, however the only non-chat model they currently support is gpt-3.5-turbo-instruct:

https://platform.openai.com/docs/models/gpt-3-5-turbo

I agree a bit more funneling there would be helpful. Can have a look soon.

@scottforte
Copy link
Contributor Author

Ok - FYI The PR here LLM (non-chat) works with gpt-3.5-turbo-0125

@jacoblee93
Copy link
Collaborator

Yes, however it's a very old legacy flow (we coerce it to use the chat endpoint behind the scenes) and shouldn't be used. I'll flag this with the rest of the team!

@scottforte
Copy link
Contributor Author

Super helpful insight - thank you

@jacoblee93
Copy link
Collaborator

Merged #5718 to help folks in the future.

@jacoblee93 jacoblee93 changed the title Fixed to match new apiKey field openai[patch],azure-openai[patch]: Update legacy classes to respect the apiKey field Jun 11, 2024
@jacoblee93
Copy link
Collaborator

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:improvement Medium size change to existing code to handle new use-cases size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants